Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DataGrid] All selectors accept only apiRef as first argument #16198

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

arminmeh
Copy link
Contributor

@arminmeh arminmeh commented Jan 15, 2025

Closes #11440

@mui-bot
Copy link

mui-bot commented Jan 15, 2025

Deploy preview: https://deploy-preview-16198--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 5d76140

Comment on lines -46 to +48
const loadingSelector = (state: GridStatePremium) => state.dataSource.loading[id] ?? false;
const errorSelector = (state: GridStatePremium) => state.dataSource.errors[id];
const loadingSelector = (apiRefObject: React.RefObject<GridApiPremium>) =>
apiRefObject.current.state.dataSource.loading[id] ?? false;
const errorSelector = (apiRefObject: React.RefObject<GridApiPremium>) =>
apiRefObject.current.state.dataSource.errors[id];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should probably be selectors with arguments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And rowSelector below as well.

Comment on lines -4 to +5
export const gridAggregationStateSelector = (state: GridStatePremium) => state.aggregation;
export const gridAggregationStateSelector = (apiRef: React.RefObject<GridApiPremium>) =>
apiRef.current.state.aggregation;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if creating a createStateSelector that pre-fetches apiRef.current.state could be useful for readability.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a type like GridApiRef*** to make React.RefObject<GridApi***> more compact could also be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to do that, but have to resolve rendering issues first 🙂

Comment on lines 229 to 231
const getPinnedColumns = React.useCallback<GridColumnPinningApi['getPinnedColumns']>(() => {
return gridPinnedColumnsSelector(apiRef.current.state);
return gridPinnedColumnsSelector(apiRef);
}, [apiRef]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should deprecate API methods that only call a selector.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please 👍🏻

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 20, 2025
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[datagrid] Refactor: make all selectors accept the same argument
4 participants